fix: backport error handling fixes to existing connectors#47
fix: backport error handling fixes to existing connectors#47hallvictoria wants to merge 2 commits into
Conversation
…into hallvictoria/add-error-handling
daviburg
left a comment
daviburg
left a comment
There was a problem hiding this comment.
[Dobby] Review comments from this session. Model used: GitHub Copilot.
I checked the PR content against the committed BPM generator on origin/master (642cbe383e). The main issue is generated-code provenance: several PR changes do not match what the committed BPM generator can emit.
Additional finding that is not cleanly inline-anchorable: GENERATION.md documents --directClient --python, but BPM origin/master does not parse a --python flag. The committed CLI supports --directClient --language=python and the legacy --pythonDirectClient; with --directClient --python, ParseDirectClientLanguage defaults to C#. Please update all five examples in this file so the documented regeneration command actually produces Python output.
| ) | ||
|
|
||
| response = await self.http_client.send_async("POST", path, body=input) | ||
| response = response = await self.http_client.send_async("POST", path, body=input) |
There was a problem hiding this comment.
[Dobby] Model used: GitHub Copilot. This duplicated assignment (response = response = ...) is valid Python, so syntax/CI can miss it, but it does not appear to be emitted by the committed BPM generator on origin/master. Please fix the generator/template that produced this, regenerate, and add a guard if this class of generated typo is expected to recur.
|
|
||
| await self.http_client.send_async("PATCH", path, body=input) | ||
| response = await self.http_client.send_async("PATCH", path, body=input) | ||
| if response.status >= 400: |
There was a problem hiding this comment.
[Dobby] Model used: GitHub Copilot. This status-check shape (response.status >= 400) does not match the committed BPM Python emitter, which emits if not (200 <= response.status < 300) for response-returning methods, and still emits unchecked await self.http_client.send_async(...) for methods without responses. I found this stale/manual-looking pattern in 14 places in office365.py. More importantly, the void-operation checks added across this PR cannot be reproduced from BPM origin/master; please land/reference the generator commit that produces these files, then regenerate so future codegen does not drop the fix.
| raise ConnectorException( | ||
| f"Operation failed with status {response.status}: {await response.text()}" | ||
| ) |
There was a problem hiding this comment.
[Dobby] Model used: GitHub Copilot. This example uses ConnectorException(f"Operation failed..."), but the SDK exception constructor requires (method, path, status_code, response_body), and the generated files use that four-argument shape. Please update the docs to match the runtime contract so future generator fixes do not copy an invalid exception pattern.
The model used was GPT-5.5 |
2 participants
Summary
Comprehensive error handling improvements across 14 connector clients with +205 new tests, source bug fixes, and code quality improvements.
Fixes #38, #39
Changes
Fixed
respond_to_event_asyncmethods (mypy error)ConnectorExceptionsignature in 2 webhook methods (used keyword args instead of positional)Changed
Updated
README.md— Updated test counts for 14 connectors (+205 tests total)CHANGELOG.md— Documented changes under[Unreleased]Testing
pytest)flake8 src/ tests/)Checklist
src/azure/connectors/*.py(unless regenerated)